Added lazy sync functionality #279

fritzduchardt · 2023-08-07T07:12:07Z

Implementation for: https://github.com/carvel-dev/carvel/tree/develop/proposals/vendir/001-lazy-synching-on-stable-config
Closes #278

…has changed since the last sync. Added extra field to lock file in order to keep track of config changes. Signed-off-by: Fritz Durchardt <[email protected]>

Signed-off-by: Fritz Durchardt <[email protected]>

kumaritanushree · 2023-08-23T09:42:06Z

pkg/vendir/cmd/sync.go

@@ -50,6 +51,7 @@ func NewSyncCmd(o *SyncOptions) *cobra.Command {

 	cmd.Flags().StringSliceVarP(&o.Directories, "directory", "d", nil, "Sync specific directory (format: dir/sub-dir[=local-dir])")
 	cmd.Flags().BoolVarP(&o.Locked, "locked", "l", false, "Consult lock file to pull exact references (e.g. use git sha instead of branch name)")
+	cmd.Flags().BoolVar(&o.Eager, "eager", false, "Ignore lazy setting in vendir yml and eagerly fetch all remote content")


Did we decide to have default functionality of vendir to fetch only if vendir.yaml has changed else ignore?

The proposal is already open. I think the default will remain unchanged, but if you have this new feature enabled via vendir.yaml config, you can opt out by setting the eager flag.

@kumaritanushree the default behavior will not change - vendir will always fetch. However, if the user sets lazy to true in the vendir.yaml for a particular content, lazy fetching will apply. The global flag serves the purpose to force a fetch, even if lazy is set in vendir.yaml

pkg/vendir/directory/directory.go

Signed-off-by: Fritz Duchardt <[email protected]>

kumaritanushree · 2023-10-11T09:15:02Z

@fritzduchardt
I have gone through the functionality and looks good but flag --lazy somehow creating confusion for me. As we say in flag description it will override "lazy" value in directory content configuration.

But if we have vendir.yml:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: sponnet
  contents:
  - path: .
    lazy: false
    http:
      url: https://github.com/spinnaker/sponnet/archive/refs/tags/v1.26.0.tar.gz

And we run just vendir sync on this it will not override this value. Also it does not make much sense to use such config but if will just look into flag --lazy it sounds the same.

Here --lazy will only update "lazy" value false not vice versa and description does not tell that.
I am not able to think better description as of now.
@joaopapereira any suggestion on this?

Zebradil · 2023-10-11T15:49:32Z

If I read the proposal correctly, lazy is false by default in the config. It can be explicitly changed to true. To disable the "lazy" from CLI, --lazy=false has to be used.

So, it wouldn't make much sense to use lazy: false in the config because it's the default. Even if it is used, --lazy=false won't affect the corresponding directories, they will be synced "non-lazily".

Does that make sense?

joaopapereira · 2023-10-11T21:21:21Z

I think that @Zebradil answer make sense

kumaritanushree · 2023-10-13T05:59:20Z

LGTM

joaopapereira

I made some comments in the PR. Also, can we add some end-to-end tests to ensure we are doing the correct thing when using the lazy flag?

joaopapereira · 2023-10-13T15:03:43Z

pkg/vendir/directory/directory.go

 	"fmt"
+	"gopkg.in/yaml.v3"


Can we use sigs.k8s.io/yaml instead? Just to keep it consistent

joaopapereira · 2023-10-13T15:04:07Z

go.mod

@@ -27,6 +27,8 @@ require (
 	sigs.k8s.io/yaml v1.3.0
 )

+require gopkg.in/yaml.v3 v3.0.1


When this becomes indirect again, please let us revert this change

joaopapereira · 2023-10-13T15:07:01Z

pkg/vendir/cmd/sync.go

@@ -50,6 +51,7 @@ func NewSyncCmd(o *SyncOptions) *cobra.Command {

 	cmd.Flags().StringSliceVarP(&o.Directories, "directory", "d", nil, "Sync specific directory (format: dir/sub-dir[=local-dir])")
 	cmd.Flags().BoolVarP(&o.Locked, "locked", "l", false, "Consult lock file to pull exact references (e.g. use git sha instead of branch name)")
+	cmd.Flags().BoolVar(&o.Lazy, "lazy", true, "Override \"lazy\" value in directory content configuration")


Maybe we can change the text here to something like:
"When this flag is set to 'false' it ignores the 'lazy' value in the directory content configuration"
Do you think it is easier to understand what the flag does?

I was thinking along the same lines. It it less vague about the purpose of this option.

Hi @joaopapereira This CLI option 'lazy' (default value is true) is really confusing. With vendir 0.37.0, vendir sync --lazy=true won't do lazy fetch. This behavior does not comply with the proposal:
https://github.com/carvel-dev/carvel/tree/develop/proposals/vendir/001-lazy-synching-on-stable-config#other-approaches-considered

This proposal should mean the CLI option --lazy default to false, and if --lazy is set to true, vendir will do lazy fetch,
Also this PR defines 'Lazy bool' as a bool instead of *bool, so it can not be used to override the CLI option '--lazy' because it always has a default value 'false'.

type DirectoryContents struct { Path string `json:"path"` Lazy bool `json:"lazy,omitempty"`

@jessehu The proposal was:

to add the lazy option to vendir.yml to allow skipping sync when the config is not changed,

to add the --lazy CLI argument to temporarily disable the effect of the lazy option.

What you reference to is a considered approach that could've satisfied the use case but wasn't accepted.

I see. Thanks. However this 'other approach considered' makes more sense. The current code behaves a little weird:

vendir sync --lazy=true won't do lazy fetch (--lazy default to true) since it does not overrites in lazy setting in vendir.yml

vendir sync --lazy=false won't do lazy fetch since it overrites in lazy setting in vendir.yml

According to this behaviour, this CLI flag should be named ignoreLazySetting or noLazy rather than lazy (the setting name in verdir.yml)

cmd.Flags().BoolVar(&o.Lazy, "lazy", true, "Set to 'false' it ignores the 'lazy' flag in the directory content configuration")

The alternative approach is less flexible, as it doesn't allow configuring "laziness" per directory in the vendir.yml configuration file.

The selected name of the CLI flag is a little confusing, I agree. It might've been more explicit to use, as you mentioned, --noLazy or alike instead of --lazy=false.

joaopapereira · 2023-10-13T15:15:11Z

pkg/vendir/directory/directory.go

@@ -243,3 +278,17 @@ func maybeChmod(path string, potentialPerms ...*os.FileMode) error {

 	return nil
 }
+
+func handleLazySync(oldConfigDigest string, newConfigDigest string, fetchLazyGlobalOverride bool, fetchLazy bool) (bool, bool) {


We try to keep functions that are only used in a single struct internal to the structure even if they do not use the structure itself. That way we will know the scope in which we expect these functions to run.
With that in mind do you mind adding this function to the Directory struct?

Not at all - done

joaopapereira · 2023-10-13T15:22:52Z

pkg/vendir/directory/directory_test.go

+
+import "testing"
+
+func Test_handleLazySync(t *testing.T) {


I am not a fan of testing not exported functions. I understand here that this code would be more complex than testing from the outside. Is there any refactoring that can make this easier?

No worries, let's remove it and use the e2e test instead

Signed-off-by: Fritz Duchardt <[email protected]>

joaopapereira

Added a comment about the added test.
After that is solved I think we are good to merge this PR

joaopapereira · 2023-10-17T16:21:36Z

test/e2e/example_lazy_test.go

+
+package e2e
+
+import (


We do have require package as a dependency, and we are using it to do all the assertions in the other tests, you can see an example here.
Do you mind refactoring this test to make it similar to the other tests?

Not at all - look much more concise now

Signed-off-by: Fritz Duchardt <[email protected]>

joaopapereira

LGTM

The option was proposed in [1] and implemented in [2]. [1]: https://github.com/carvel-dev/carvel/blob/develop/proposals/vendir/001-lazy-synching-on-stable-config/README.md [2]: carvel-dev/vendir#279

The option was proposed in [1] and implemented in [2]. [1]: https://github.com/carvel-dev/carvel/blob/develop/proposals/vendir/001-lazy-synching-on-stable-config/README.md [2]: carvel-dev/vendir#279 Signed-off-by: German Lashevich <[email protected]>

feat: added lazy sync functionality to only do a sync, if the config …

5d8f4d2

…has changed since the last sync. Added extra field to lock file in order to keep track of config changes. Signed-off-by: Fritz Durchardt <[email protected]>

fritzduchardt force-pushed the added_lazy_syncing branch from edd9918 to 5d8f4d2 Compare August 7, 2023 07:17

Fritz Durchardt added 2 commits August 10, 2023 13:31

feat: reimplementation of lazy sync functionality

dca8107

Signed-off-by: Fritz Durchardt <[email protected]>

feat: first working draft

6fa6535

Signed-off-by: Fritz Durchardt <[email protected]>

fritzduchardt changed the title ~~feat: added lazy sync functionality~~ Draft: added lazy sync functionality Aug 10, 2023

Remove hash container

77b9440

Signed-off-by: Fritz Durchardt <[email protected]>

kumaritanushree reviewed Aug 23, 2023

View reviewed changes

pkg/vendir/directory/directory.go Outdated Show resolved Hide resolved

fritzduchardt added 3 commits October 8, 2023 11:15

Revamp of entire lazy sync logic

a1beab7

Signed-off-by: Fritz Duchardt <[email protected]>

Tweaked handling of lock file

e0a6b99

Signed-off-by: Fritz Duchardt <[email protected]>

Change naming

c004cac

Signed-off-by: Fritz Duchardt <[email protected]>

fritzduchardt requested a review from kumaritanushree October 8, 2023 13:53

fritzduchardt changed the title ~~Draft: added lazy sync functionality~~ Added lazy sync functionality Oct 10, 2023

Zebradil mentioned this pull request Oct 11, 2023

Sync Cache that relies on release versions #274

Closed

joaopapereira requested changes Oct 13, 2023

View reviewed changes

fritzduchardt added 2 commits October 15, 2023 12:08

Added e2e test, removed unit test, improved help message.

622b00c

Signed-off-by: Fritz Duchardt <[email protected]>

Some more comments

e9af3ba

Signed-off-by: Fritz Duchardt <[email protected]>

fritzduchardt requested a review from joaopapereira October 15, 2023 11:01

joaopapereira requested changes Oct 17, 2023

View reviewed changes

Introduced require test function in e2e test

82c3c9f

Signed-off-by: Fritz Duchardt <[email protected]>

fritzduchardt requested a review from joaopapereira October 19, 2023 06:16

joaopapereira approved these changes Oct 26, 2023

View reviewed changes

joaopapereira merged commit 890f119 into carvel-dev:develop Oct 26, 2023
4 checks passed

Zebradil mentioned this pull request Dec 21, 2023

Document lazy option in the vendir spec carvel-dev/carvel#705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added lazy sync functionality #279

Added lazy sync functionality #279

fritzduchardt commented Aug 7, 2023 •

edited

Loading

kumaritanushree Aug 23, 2023

fritzduchardt Aug 23, 2023

fritzduchardt Oct 8, 2023

kumaritanushree commented Oct 11, 2023

Zebradil commented Oct 11, 2023

joaopapereira commented Oct 11, 2023

kumaritanushree commented Oct 13, 2023

joaopapereira left a comment

joaopapereira Oct 13, 2023

fritzduchardt Oct 15, 2023

joaopapereira Oct 13, 2023

fritzduchardt Oct 15, 2023

joaopapereira Oct 13, 2023

fritzduchardt Oct 15, 2023

jessehu Dec 3, 2023

Zebradil Dec 21, 2023

jessehu Dec 22, 2023

Zebradil Dec 22, 2023

joaopapereira Oct 13, 2023

fritzduchardt Oct 15, 2023

joaopapereira Oct 13, 2023

fritzduchardt Oct 15, 2023

joaopapereira left a comment

joaopapereira Oct 17, 2023

fritzduchardt Oct 19, 2023

joaopapereira left a comment

Added lazy sync functionality #279

Added lazy sync functionality #279

Conversation

fritzduchardt commented Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumaritanushree commented Oct 11, 2023

Zebradil commented Oct 11, 2023

joaopapereira commented Oct 11, 2023

kumaritanushree commented Oct 13, 2023

joaopapereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaopapereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaopapereira left a comment

Choose a reason for hiding this comment

fritzduchardt commented Aug 7, 2023 •

edited

Loading